Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement RPC feature #544

Merged
merged 20 commits into from
Jan 17, 2025
Merged

Implement RPC feature #544

merged 20 commits into from
Jan 17, 2025

Conversation

bcherry
Copy link
Contributor

@bcherry bcherry commented Jan 14, 2025

Pretty much identical to livekit/client-sdk-android#578

I included tests which mock the underlying data channel sending and receiving to test both sides in the same SDK app instance.

@bcherry bcherry changed the title RPC feature Implement RPC feature Jan 14, 2025
@bcherry bcherry requested a review from hiroshihorie January 14, 2025 08:43
@bcherry bcherry marked this pull request as ready for review January 14, 2025 08:43
Copy link
Member

@hiroshihorie hiroshihorie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Looks good !
Applied format & moved code to a separate file (extension) to organize.

Was just wondering if the Public APIs are "Swifty" enough 🧐

@bcherry
Copy link
Contributor Author

bcherry commented Jan 14, 2025

Was just wondering if the Public APIs are "Swifty" enough 🧐

I could make it more swifty like this: register(rpcMethod:"hello") { data in } and perform(rpcMethod: "hello", on: "some-identity", payload: "xxx", timeout: 8)?

Only downside is it loses consistency with other sdks in parameter structure. i was also thinking that maybe it would be better to revise across all SDKs to a separate construct attached to room e.g. room.rpc.register("xxx") { _ in } and room.rpc.perform("xxx", on: "yyy")

@bcherry
Copy link
Contributor Author

bcherry commented Jan 17, 2025

Was just wondering if the Public APIs are "Swifty" enough 🧐

I could make it more swifty like this: register(rpcMethod:"hello") { data in } and perform(rpcMethod: "hello", on: "some-identity", payload: "xxx", timeout: 8)?

Only downside is it loses consistency with other sdks in parameter structure. i was also thinking that maybe it would be better to revise across all SDKs to a separate construct attached to room e.g. room.rpc.register("xxx") { _ in } and room.rpc.perform("xxx", on: "yyy")

@hiroshihorie I made those changes then reverted them while writing the docs because the way the docs are written it really is a lot nicer if all platforms have the same method names

@bcherry bcherry merged commit 7b4ec4d into main Jan 17, 2025
9 of 12 checks passed
@bcherry bcherry deleted the bcherry/rpc branch January 17, 2025 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants